Skip to content

Mock bcryptjs in API tests - #326

Merged
kaareal merged 9 commits into
masterfrom
feature/password-test-bcrypt-cost-6f6c26
Oct 7, 2026
Merged

kaareal merged 9 commits into
masterfrom
feature/password-test-bcrypt-cost-6f6c26

Conversation

@kaareal

@kaareal kaareal commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

What

  • services/api/__mocks__/bcryptjs.js: wraps the real bcryptjs and only overrides genSalt to use cost 4 (bcrypt's minimum). hash and compare are the real implementations.
  • services/api/src/utils/testing/setup/mocks.js: adds vi.mock('bcryptjs') next to the other package mocks.

No production code, .env or deployment change: password.js still hashes at cost 12.

Why

setPassword hashes at cost 12 with bcryptjs (pure JS). Every createUser({ password }) and every password login in tests paid for a cost-12 hash or compare, which dominated the auth test files.

Measurements (local, warm runs)

Run Before After
pnpm test src/routes/auth/password.test.js (20 tests) 8.0 - 10.2 s 1.5 - 1.8 s
pnpm test (39 files, 399 tests) not measured 14.0 s

Reviewer notes

  • Tests still hash and compare with real bcrypt, so stored secrets are genuine $2b$04$ hashes.
  • The full suite is intermittently flaky locally independent of this change: 2 of 9 runs with the mock and 1 of 6 runs without it had failures, in different auth tests each time (otp, totp, integration, password login).
  • Earlier commits on this branch tried a BCRYPT_SALT_PASSES env variable and a fully faked mock; squash-merging leaves only the final mock.

Password hashing used a hard-coded cost of 12 with bcryptjs (pure JS),
so every createUser({ password }) and login in tests paid ~200ms.
The cost now reads PASSWORD_BCRYPT_COST, defaulting to 12, and
vitest.config.js sets it to 4 for the test run.
@github-actions

github-actions Bot commented Oct 5, 2026

Copy link
Copy Markdown

API Changes

No changes.

@kaareal
kaareal requested a review from andrewplummer October 5, 2026 14:09

@andrewplummer andrewplummer left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is totally fine - approved, however I was thinking to myself maybe mocking bcrypt is the more "correct" way ... either way it's fine

@kaareal

kaareal commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator Author

yeah i prefer the mocking too, i dont really want an env var for this, let me modify and see if thats possible

Drops BCRYPT_SALT_PASSES; password.js, .env and vitest.config.js are
back to master. Tests now use __mocks__/bcryptjs.js.
@kaareal kaareal changed the title Lower bcrypt cost in API tests Mock bcryptjs in API tests Oct 7, 2026
@kaareal
kaareal merged commit 7f061a2 into master Oct 7, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants